-
-
Notifications
You must be signed in to change notification settings - Fork 30.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
gh-125260: gzip: let compress mtime default to 0 #125261
Conversation
This needs a NEWS entry, as this is a user-facing error. Also, could you add a test for this? |
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
I have rebased it onto main now. I had probably based it off the old master branch. I would appreciate some help with the NEWS and test. I only saw Misc/NEWS.d/3.13.0a1.rst but nothing for 3.14 yet. |
@ZeroIntensity I'm not sure about backport labels here as the function works as intended (so this is a feature) -- what was your rationale? |
Hmm, is this a feature? The original issue was marked as a bug, so I thought it would be backportable. |
It was opened with the bug template, hence the bug label, but it's a judgement call as to 'bug' or 'feature'. Personally I would be hesitant to backport, but others may have different opinions. A |
I think that's right, I'll update the labels. My bad! |
3750071
to
c83107b
Compare
I have made the requested changes; please review again |
Thanks for making the requested changes! @AA-Turner: please review the changes made to this pull request. |
this follows GNU gzip, which defaults to store 0 as mtime for compressing stdin, where no file mtime is involved This makes gzip.compress(str) output deterministic by default and greatly helps reproducible builds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that looks like a great idea!
The reason why Bernhard would like to consider it as a bug (and thus backport it) is that it breaks reproducible builds. |
As AA-Turner said, "the function works as intended (so this is a feature)" and thus not a bug. Only bugs can be backported. |
@bmwiedemann please don't force-push, as it makes PRs harder to review. See the devguide for similar guidance. We squash-merge all commits. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've had to leave a couple of comments as diffs rather than suggestions due to limitations in the GH review interface -- sorry.
A
Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
A
…eZ0Mb.rst Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
Thanks @bmwiedemann! A |
this follows GNU gzip, which defaults to
store 0 as mtime for compressing stdin, where no file mtime is involved
This makes gzip.compress(str) output deterministic by default and greatly helps reproducible builds.
📚 Documentation preview 📚: https://cpython-previews--125261.org.readthedocs.build/